Support protobuf fields. by dypet · Pull Request #82 · sonic-net/sonic-dash-ha

dypet · 2025-07-08T14:24:49Z

Adding support for protobuf field values in DASH_HA_SET_CONFIG_TABLE and DASH_HA_SCOPE_CONFIG_TABLE

mssonicbld · 2025-07-08T14:24:56Z

/azp run

azure-pipelines · 2025-07-08T14:25:05Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-07-08T14:50:19Z

/azp run

azure-pipelines · 2025-07-08T14:50:29Z

Azure Pipelines successfully started running 1 pipeline(s).

yue-fred-gao · 2025-07-10T01:37:05Z

crates/hamgrd/src/db_structs.rs

@@ -207,19 +208,7 @@ pub fn now_in_millis() -> i64 {
 #[derive(Serialize, Deserialize, SonicDb)]
 #[sonicdb(table_name = "DASH_HA_SET_CONFIG_TABLE", key_separator = ":", db_name = "APPL_DB")]
 pub struct DashHaSetConfigTable {


Is it necessary to keep DashHaSetConfigTable structure with a single attribute? Can we move the derive macros to HaSetConfig?

crates/swss-common-bridge/src/consumer.rs

mssonicbld · 2025-07-10T22:13:00Z

/azp run

azure-pipelines · 2025-07-10T22:13:10Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-07-11T17:08:05Z

/azp run

azure-pipelines · 2025-07-11T17:08:15Z

Azure Pipelines successfully started running 1 pipeline(s).

yue-fred-gao

It looks good in general. Let me review it when I get back.

yue-fred-gao · 2025-07-12T01:55:38Z

crates/hamgrd/src/actors/ha_scope.rs


        let mut dpu_ha_state_state5 = make_dpu_ha_scope_state("active");
-        dpu_ha_state_state5.ha_term = "2".to_string();
+        dpu_ha_state_state5.ha_term = "active".to_string();


is ha_term a number?

mssonicbld · 2025-07-14T16:51:36Z

/azp run

azure-pipelines · 2025-07-14T16:51:45Z

Azure Pipelines successfully started running 1 pipeline(s).

yue-fred-gao · 2025-07-15T14:49:27Z

crates/hamgrd/src/actors/ha_set.rs

-            scope: dash_ha_set_config.scope.clone(),
+            vip_v4: dash_ha_set_config.vip_v4.as_ref().map(ip_to_string).unwrap_or_default(),
+            vip_v6: dash_ha_set_config.vip_v6.as_ref().map(ip_to_string),
+            owner: format!("{:?}", dash_ha_set_config.owner).into(),


We need proper conversion. I think the convention is removing prefix and changing to lower case.

yue-fred-gao · 2025-07-15T17:47:51Z

crates/hamgrd/src/actors/ha_set.rs

    use swss_common::SonicDbTable;
    use swss_common_testing::*;

+    fn protobuf_struct_to_json<T: prost::Message + Default + serde::Serialize>(cfg: &T) -> HashMap<String, CxxString> {


The name is _to_json but actually it returns kfv

yue-fred-gao · 2025-07-15T17:55:42Z

crates/hamgrd/src/actors/ha_scope.rs


 // Implements internal action functions for HaScopeActor
 impl HaScopeActor {
+    fn desired_ha_state_to_ha_role(desired_ha_state: i32) -> String {


I think it'd be better to move this function to sonic-dash-api-proto/src/lib.rs for helper functions around protobuf types

yue-fred-gao · 2025-07-15T18:00:49Z

crates/hamgrd/src/actors/ha_scope.rs

-            send! { key: DashHaScopeConfigTable::table_name(), data: { "key": &scope_id, "operation": "Set",
-                    "field_values": {"version": "1", "disable": "true", "desired_ha_state": "active", "approved_pending_operation_ids": "" },
+            send! { key: HaScopeConfig::table_name(), data: { "key": &scope_id, "operation": "Set",
+                    "field_values": {"json": "{\"version\":\"1\",\"disabled\":true,\"desired_ha_state\":2,\"approved_pending_operation_ids\":[]}"},


Let's not use enum value here and below.

yue-fred-gao · 2025-07-15T18:10:26Z

crates/hamgrd/src/db_structs.rs

    Err(anyhow::anyhow!("DPU entry not found for slot {}", dpu_id))
 }

+pub fn ip_to_string(ip: &IpAddress) -> String {


move to sonic-dash-api-proto crate

yue-fred-gao · 2025-07-15T18:13:30Z

crates/hamgrd/src/db_structs.rs

+    }
+}
+
+pub fn decode_from_json_string<T: for<'de> serde::Deserialize<'de>>(


Is it better to move decode/encode function for protobuf objects to sonic-dash-api-proto? And the name should be from_field_values

yue-fred-gao · 2025-07-15T18:17:25Z

crates/sonic-dash-api-proto/build.rs

+
+    proto_config.type_attribute(
+        "dash.ha_scope_config.HaScopeConfig",
+        "#[allow(unused_imports)] use sonicdb_derive::SonicDb;",


why allow(unused_imports)?

yue-fred-gao · 2025-07-15T18:41:43Z

crates/hamgrd/src/actors/ha_scope.rs

 impl HaScopeActor {
+    fn desired_ha_state_to_ha_role(desired_ha_state: i32) -> String {
+        match DesiredHaState::try_from(desired_ha_state) {
+            Ok(DesiredHaState::HaStateActive) => "active".to_string(),


Why the enum name is DesiredHaState not HaState based on protobuf? If HaState is used in another field, will it generate a different enum type in rust?
Can we use strum::Display to generate this function automatically?

I see protobuf defines DesiredHaState type

Jing will change the enums to follow proper naming convention: sonic-net/sonic-dash-api#42. With the change, I believe Prost will generate rust enum value without the common prefix. Then you can use strum::Display to generate above mapping.

yue-fred-gao · 2025-07-15T18:52:12Z

crates/sonicdb-derive/src/lib.rs

+                true
+            }
+
+            fn convert_pb_to_json(kfv: &mut KeyOpFieldValues) {


Can this function be implemented in the trait instead of generated by macro? If is_proto returns false, it can do nothing as the default implementation. Then you don't fold expanded_proto into expanded to avoid duplicate code.

mssonicbld · 2025-07-15T21:37:09Z

/azp run

azure-pipelines · 2025-07-15T21:37:18Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-07-16T15:04:31Z

/azp run

azure-pipelines · 2025-07-16T15:04:41Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-07-16T16:23:40Z

/azp run

azure-pipelines · 2025-07-16T16:23:50Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2025-07-22T19:09:43Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-07-24T17:35:37Z

/azp run

azure-pipelines · 2025-07-24T17:35:47Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-07-31T16:07:56Z

/azp run

azure-pipelines · 2025-07-31T16:08:05Z

Azure Pipelines successfully started running 1 pipeline(s).

Moving SonicDbTable to sonic-dash-ha as part of sonic-net/sonic-dash-ha#82

mssonicbld · 2025-08-27T02:13:23Z

Cherry-pick PR to msft-202506: Azure/sonic-dash-ha.msft#13

~~Adding is_proto and convert_pb_to_json to SonicDbTable to support the changes in sonic-net/sonic-dash-ha#82 Moving SonicDbTable to sonic-dash-ha as part of sonic-net/sonic-dash-ha#82

Support protobuf fields.

b5b164d

Add submodule.

fea7117

yue-fred-gao reviewed Jul 10, 2025

View reviewed changes

dypet added 2 commits July 10, 2025 11:28

Use protobuf struct directly.

23e9318

encode/decode to single json string.

a18a3b0

Use trait instead of matching on table name.

c2f2903

dypet requested a review from yue-fred-gao July 11, 2025 17:09

yue-fred-gao reviewed Jul 12, 2025

View reviewed changes

testcase update.

ac35e67

yue-fred-gao reviewed Jul 15, 2025

View reviewed changes

Update submodule in azure pipeline.

9663425

update submodule in pipeline.

44f701e

Merge remote-tracking branch 'upstream/master' into support_protobuf

f2b34a7

fix formatting.

623f11f

remove swss-common changes.

f385578

dypet added 3 commits July 31, 2025 10:38

Merge remote-tracking branch 'upstream/master' into support_protobuf

9427cf0

fix merge conflict.

7663956

Move SonicDbTable to sonic-common crate.

da1ae9d

mssonicbld added the Cherry Pick Conflict_msft-202506 label Jul 31, 2025

dypet requested a review from yue-fred-gao July 31, 2025 17:40

yue-fred-gao approved these changes Jul 31, 2025

View reviewed changes

yue-fred-gao merged commit cfcc624 into sonic-net:master Jul 31, 2025
3 checks passed

qiluo-msft pushed a commit to sonic-net/sonic-swss-common that referenced this pull request Jul 31, 2025

Move SonicDbTable to sonic-dash-ha. (#1056)

fc35dd0

Moving SonicDbTable to sonic-dash-ha as part of sonic-net/sonic-dash-ha#82

wen587 pushed a commit to wen587/sonic-swss-common that referenced this pull request Aug 5, 2025

Move SonicDbTable to sonic-dash-ha. (sonic-net#1056)

56112c1

Moving SonicDbTable to sonic-dash-ha as part of sonic-net/sonic-dash-ha#82

prabhataravind added Request for msft-202506 Branch Approved for msft-202506 Branch and removed Request for msft-202506 Branch Cherry Pick Conflict_msft-202506 labels Aug 27, 2025

mssonicbld mentioned this pull request Aug 27, 2025

[action] [PR:82] Support protobuf fields. Azure/sonic-dash-ha.msft#13

Merged

mssonicbld added the Created PR to msft-202506 Branch label Aug 27, 2025

mssonicbld mentioned this pull request Aug 27, 2025

[action] [PR:1056] Move SonicDbTable to sonic-dash-ha. Azure/sonic-swss-common.msft#51

Merged

mssonicbld added Included in msft-202506 Branch and removed Created PR to msft-202506 Branch labels Aug 27, 2025

mssonicbld mentioned this pull request Sep 10, 2025

[action] [PR:1056] Move SonicDbTable to sonic-dash-ha. Azure/sonic-swss-common.msft#65

Merged

Conversation

dypet commented Jul 8, 2025

Uh oh!

mssonicbld commented Jul 8, 2025

Uh oh!

azure-pipelines bot commented Jul 8, 2025

Uh oh!

mssonicbld commented Jul 8, 2025

Uh oh!

azure-pipelines bot commented Jul 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mssonicbld commented Jul 10, 2025

Uh oh!

azure-pipelines bot commented Jul 10, 2025

Uh oh!

mssonicbld commented Jul 11, 2025

Uh oh!

azure-pipelines bot commented Jul 11, 2025

Uh oh!

yue-fred-gao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Jul 14, 2025

Uh oh!

azure-pipelines bot commented Jul 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Jul 15, 2025

Uh oh!

azure-pipelines bot commented Jul 15, 2025

Uh oh!